Fixed errors occurring when using certain web framework integration packages and MySQL options simultaneously in @fedify/init#656
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the @fedify/init package to streamline environment variable management and task definitions across various web frameworks. It introduces a centralized env property in the WebFrameworkInitializer interface, automates the inclusion of @std/dotenv for Deno projects, and refactors framework tasks into a more maintainable constant-based structure. The review feedback recommends strengthening type safety by using Record<string, string> instead of object for the env and tasks properties and fixing a minor quoting typo in the JSDoc for RuntimeDescription.
| /** Environment variables required by this framework, keyed by name to | ||
| * default value. Merged together with KV store and message queue env vars | ||
| * into the generated `.env` file. */ | ||
| env?: object; |
There was a problem hiding this comment.
The env property should be typed as Record<string, string> instead of object to provide better type safety and ensure that environment variables are always key-value pairs of strings.
| env?: object; | |
| env?: Record<string, string>; |
There was a problem hiding this comment.
The properties for this option can vary depending on other conditions. Using a Record<string, string> type could cause type errors and confuse contributors, so I used object instead.
There was a problem hiding this comment.
What kind of types are these properties filled with?
There was a problem hiding this comment.
Currently, the HOST property is used in nitroDescription when in test mode. It is set as testMode ? { HOST: "127.0.0.1" } : {}. If that property type is defined as Record<string, string>, a type error occurs on the entire result of the init method. This requires unnecessary type assertions. It could also cause confusion for contributors. I left a mention about a similar issue in another comment.
2026-04-05.17.53.55.mov
There was a problem hiding this comment.
Hmm, could you show the whole type error message? I guess we could solve this somehow.
There was a problem hiding this comment.
TS2322 [ERROR]: Type 'Promise<{ command: string[]; dependencies: { "@fedify/lint"?: string | undefined; "@fedify/h3": string; }; devDependencies: { "@fedify/lint": string; eslint: string; "@biomejs/biome": string; }; federationFile: string; ... 4 more ...; instruction: Message; }>' is not assignable to type 'WebFrameworkInitializer | Promise<WebFrameworkInitializer>'.
Type 'Promise<{ command: string[]; dependencies: { "@fedify/lint"?: string | undefined; "@fedify/h3": string; }; devDependencies: { "@fedify/lint": string; eslint: string; "@biomejs/biome": string; }; federationFile: string; ... 4 more ...; instruction: Message; }>' is not assignable to type 'Promise<WebFrameworkInitializer>'.
Type '{ command: string[]; dependencies: { "@fedify/lint"?: string | undefined; "@fedify/h3": string; }; devDependencies: { "@fedify/lint": string; eslint: string; "@biomejs/biome": string; }; federationFile: string; ... 4 more ...; instruction: Message; }' is not assignable to type 'WebFrameworkInitializer'.
Types of property 'env' are incompatible.
Type '{ HOST: string; } | { HOST?: undefined; }' is not assignable to type 'Record<string, string> | undefined'.
Type '{ HOST?: undefined; }' is not assignable to type 'Record<string, string>'.
Property 'HOST' is incompatible with index signature.
Type 'undefined' is not assignable to type 'string'.
init: async ({ packageManager: pm, testMode }) => ({
^
at file:///workspaces/fedify/packages/init/src/webframeworks/nitro.ts:11:53
The expected type comes from the return type of this signature.
init(
^
at file:///workspaces/fedify/packages/init/src/types.ts:114:3
There was a problem hiding this comment.
How about passing undefined instead of {}?
testMode ? { HOST: "127.0.0.1" } : undefinedOr using type assertion?
testMode ? { HOST: "127.0.0.1" } : ({} as Record<string, string>)There was a problem hiding this comment.
I considered the future use of this property in deinitions of WebFrameworkDescription object of other framework. Since using undefined can be only used in cases exist or not, using type assertion would be appropriate. However, when the required properties differ, the type must be explicitly specified as as Record<string, string> every time. Doing this unnecessary type definition is a very tedious task.
For env, since it is not widely used yet, one might think it's fine to specify it manually each time. However, for tasks, this explicit specification is required every time. For tasks in a framework that supports both Deno and Node.js, at least the following properties are needed: pm === "deno" ? { dev: "deno task" } : { dev: "node script", lint: "eslint ." } Deno does not require lint task cause it has its own lint command. However, Node.js requires lint script definition. Therefore, since the properties of the two objects are different, specifying as Record<string, string> is always necessary.
If env also gets used in many WebFrameworkDescription object definitions like tasks, as Record<string, string> will be required every time. There is no need to confuse contributors by forcing such unnecessary type specifications. For this reason, I defined the types of the two properties as object instead of Record<string, string>.
2chanhaeng
changed the title
fedify init@fedify/init
issues-auto-labeler
bot
added
activitypub/interop
|
@codex review |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnv handling added to init: frameworks may supply an Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (fedify init)"
participant Action as "Init Action Layer"
participant Templates as "Template Generator"
participant FS as "Filesystem / Output"
CLI->>Action: init(packageManager, initializer, env, kv, mq)
Action->>Action: compute dependencies (include `@std/dotenv` if packageManager==deno && env non-empty)
Action->>Templates: request imports/files (pass packageManager, env, kv, mq, initializer)
Templates->>Action: return files & import blocks (may include dotenv load)
Action->>FS: write scaffold (files, tasks, deps)
FS-->>CLI: project scaffolded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/init/src/types.ts`:
- Around line 87-90: The types file defines env and tasks as type object which
is too permissive; change the declarations for env and tasks in the types.ts
(the properties named env and tasks) to be Record<string, string> so they match
downstream consumers (InitCommandData.env and package.json scripts) and prevent
arrays/nested objects from passing type checks; update any related
interface/type definitions that reference these properties to use Record<string,
string> as well.
In `@packages/init/src/webframeworks/bare-bones.ts`:
- Around line 67-79: TASKS currently defines dev/prod for runtimes but omits a
lint entry for non-Deno targets; update the TASKS object (the TASKS constant) to
add a "lint" script for both "bun" and "node" entries that runs the ESLint
config generated for these projects (e.g., something like an appropriate eslint
command that targets the project), so bun and node have a standard lint entry
analogous to other initializers; ensure the "deno" entry remains unchanged.
In `@packages/init/src/webframeworks/elysia.ts`:
- Around line 80-84: The "start" npm script currently launches the built app
without dotenvx so production .env vars are not loaded; update the node.scripts
"start" entry so it runs under dotenvx (same pattern as "dev") — e.g., invoke
dotenvx run -- before the node command and preserve NODE_ENV=production —
ensuring the "start" script loads environment variables like MySQL credentials
at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ccd4129-54e1-423c-9a23-6e09318700a4
📒 Files selected for processing (17)
mise.tomlpackages/init/src/action/deps.tspackages/init/src/action/patch.tspackages/init/src/action/set.tspackages/init/src/action/templates.tspackages/init/src/templates/bare-bones/main/deno.ts.tplpackages/init/src/templates/hono/index/deno.ts.tplpackages/init/src/types.tspackages/init/src/webframeworks/astro.tspackages/init/src/webframeworks/bare-bones.tspackages/init/src/webframeworks/elysia.tspackages/init/src/webframeworks/express.tspackages/init/src/webframeworks/hono.tspackages/init/src/webframeworks/next.tspackages/init/src/webframeworks/nitro.tspackages/init/src/webframeworks/solidstart.tspackages/init/src/webframeworks/utils.ts
💤 Files with no reviewable changes (2)
- packages/init/src/templates/hono/index/deno.ts.tpl
- packages/init/src/templates/bare-bones/main/deno.ts.tpl
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This reverts commit 15f56a8.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/init/src/action/deps.ts`:
- Around line 29-40: Introduce a small shared helper (e.g.,
needsDenoDotenv(packageManager, env) or shouldIncludeStdDotenv) that
encapsulates the predicate packageManager === "deno" && Object.keys(env).length
> 0, then replace the inline predicate in the Deps builder (the object that
currently spreads { "@std/dotenv": deps["@std/dotenv"] }) and the analogous
check in packages/init/src/action/templates.ts to call that helper so both
locations use the same logic and cannot drift.
In `@packages/init/src/webframeworks/solidstart.ts`:
- Around line 98-102: The package scripts under the node object define a start
task that doesn't load .env in production; update the node.start script (the
"start" entry in the node object) to invoke the process with environment file
loading (e.g., use the existing dotenvx pattern: run the start via "dotenvx run
-- vinxi start" or equivalent) so that environment variables required by
packages like `@fedify/mysql` are available when running vinxi start in
production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c27b575-8942-4876-8520-27821d544b1a
📒 Files selected for processing (17)
mise.tomlpackages/init/src/action/deps.tspackages/init/src/action/patch.tspackages/init/src/action/set.tspackages/init/src/action/templates.tspackages/init/src/templates/bare-bones/main/deno.ts.tplpackages/init/src/templates/hono/index/deno.ts.tplpackages/init/src/types.tspackages/init/src/webframeworks/astro.tspackages/init/src/webframeworks/bare-bones.tspackages/init/src/webframeworks/elysia.tspackages/init/src/webframeworks/express.tspackages/init/src/webframeworks/hono.tspackages/init/src/webframeworks/next.tspackages/init/src/webframeworks/nitro.tspackages/init/src/webframeworks/solidstart.tspackages/init/src/webframeworks/utils.ts
💤 Files with no reviewable changes (2)
- packages/init/src/templates/bare-bones/main/deno.ts.tpl
- packages/init/src/templates/hono/index/deno.ts.tpl
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/init/src/action/templates.ts`:
- Around line 65-68: The concat call inside the when branch is passing a string
(which iterates into chars) to `@fxts/core/concat`, so the import statement is
being spread into individual characters; update the when(...) call that
references needsDenoDotenv({ packageManager, env }) to call concat with an array
containing the full import string (e.g., change concat('import
"@std/dotenv/load";') to concat(['import "@std/dotenv/load";'])) so the whole
import line is inserted as a single element.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09e2756c-a0a8-48d5-9628-66b26f40eab2
📒 Files selected for processing (5)
packages/init/src/action/deps.tspackages/init/src/action/templates.tspackages/init/src/action/utils.tspackages/init/src/webframeworks/bare-bones.tspackages/init/src/webframeworks/solidstart.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/init/src/action/templates.ts`:
- Around line 49-52: Update the JSDoc for the getImports function to reflect its
current parameters: kv, mq, packageManager, and env (typed by InitCommandData).
Edit the docblock above export const getImports to list and briefly describe
each destructured param (kv, mq, packageManager, env) and update the `@param`
annotation to reference the full destructured object (e.g., `@param` param0 -
Destructured InitCommandData containing kv, mq, packageManager, and env). Ensure
the `@returns` description remains accurate for the function's return value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d052db82-e898-4586-96b8-b3ea9494bda9
📒 Files selected for processing (1)
packages/init/src/action/templates.ts
2 participants
Summary
Fixed an issue where
fedify initfailed to properly create projects when using specific web frameworks (Astro, ElysiaJS, Nitro) alongside@fedify/mysql. Most of these issues were configuration problems during app creation with@fedify/initrather than bugs within@fedify/mysqlitself.Related issue
@fedify/mysql#649Changes
envproperty toWebFrameworkInitializerso frameworks can declaretheir own environment variables, merged with KV/MQ env vars into
.env.@std/dotenv/loadonly when env vars exist (Deno),removing hardcoded imports from bare-bones and hono templates.
@dotenvx/dotenvxdependency for Astro on Node.js and wrappedNode.js task commands with
dotenvx run --where missing.--allow-readto Deno task commands for Express and ElysiaJSso
.envfiles can be read.TASKSlookup tablesper framework for readability.
packageManagerToRuntimetopmToRt.depends = ["prepare"]totest:inittask in mise.toml.Benefits
.envvariables are now properly loaded at runtime, fixing theTypeError: Cannot read properties of undefinedinmysql2..envfiles.Checklist
Did you add a changelog entry to the CHANGES.md?Did you write some relevant docs about this change (if it's a new feature)?Did you write some tests for this change (if it's a new feature)?mise teston your machine?